Closed
Conversation
WalkthroughReplaces Mocha+c8 with Vitest for testing and coverage in packages/root-utils/package.json (scripts and devDependencies updated). Updates dependencies Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EvanHahn
force-pushed
the
convert-rootutils-package-to-vitest
branch
2 times, most recently
from
3449b6f to
6547e0f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/root-utils/test/root-utils.test.js (1)
46-57: Move setup into the guarded block to prevent test-state leaks.Lines 49-50 run before
try, so a setup failure can leavecurrentbehind and make later tests flaky.Safer setup/teardown shape
- fs.mkdirSync(currentDirectory); - fs.closeSync(fs.openSync(packageJSONPath, 'w')); - - try { + try { + fs.mkdirSync(currentDirectory); + fs.closeSync(fs.openSync(packageJSONPath, 'w')); expect(getProcessRoot()).toBe(currentDirectory); } finally { - fs.unlinkSync(packageJSONPath); - fs.rmdirSync(currentDirectory); + if (fs.existsSync(packageJSONPath)) { + fs.unlinkSync(packageJSONPath); + } + if (fs.existsSync(currentDirectory)) { + fs.rmdirSync(currentDirectory); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/root-utils/test/root-utils.test.js` around lines 46 - 57, The test creates currentDirectory and packageJSONPath before entering the try/finally, risking leftover files if setup fails; move the setup (fs.mkdirSync and creating the package.json file) into the try block immediately before the expect so that cleanup in the finally always runs even on setup failure, referencing the variables currentDirectory and packageJSONPath and the tested function getProcessRoot() to keep teardown (fs.unlinkSync and fs.rmdirSync) paired with creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/root-utils/test/root-utils.test.js`:
- Around line 27-29: The current test only checks that a package.json exists at
getCallerRoot(), which can be satisfied by an unrelated root; instead read and
parse the package.json at path.join(callerRoot, 'package.json') and assert its
"name" matches the package name for this module (load the expected package name
by requiring the package.json adjacent to the test), e.g. const pkg =
JSON.parse(fs.readFileSync(path.join(callerRoot,'package.json'),'utf8'));
expect(pkg.name).toBe(require('../package.json').name); this strengthens the
getCallerRoot() assertion and prevents false positives.
- Line 69: Replace the permissive suffix assertion on getProcessRoot() with an
exact path equality check: locate the test in root-utils.test.js that calls
getProcessRoot() (the expect(getProcessRoot().endsWith('root-utils')).toBe(true)
line) and change it to compare the full resolved path returned by
getProcessRoot() to the exact expected path string (or to a path constructed
with path.resolve/path.join to avoid platform issues) using toBe or toEqual so
the test asserts precise equality rather than just a suffix.
---
Nitpick comments:
In `@packages/root-utils/test/root-utils.test.js`:
- Around line 46-57: The test creates currentDirectory and packageJSONPath
before entering the try/finally, risking leftover files if setup fails; move the
setup (fs.mkdirSync and creating the package.json file) into the try block
immediately before the expect so that cleanup in the finally always runs even on
setup failure, referencing the variables currentDirectory and packageJSONPath
and the tested function getProcessRoot() to keep teardown (fs.unlinkSync and
fs.rmdirSync) paired with creation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/root-utils/package.jsonpackages/root-utils/test/.eslintrc.jspackages/root-utils/test/root-utils.test.js
Comment on lines
+27
to
+29
| const callerRoot = getCallerRoot(); | ||
| expect(typeof callerRoot).toBe('string'); | ||
| expect(fs.existsSync(path.join(callerRoot, 'package.json'))).toBe(true); |
There was a problem hiding this comment.
Strengthen the getCallerRoot success assertion.
On Line 29, checking only for package.json existence can pass for unrelated roots (for example, a tool/package root), so this can miss regressions in caller-root resolution.
Suggested tightening
- expect(typeof callerRoot).toBe('string');
- expect(fs.existsSync(path.join(callerRoot, 'package.json'))).toBe(true);
+ expect(callerRoot).toBe(path.join(__dirname, '..'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const callerRoot = getCallerRoot(); | |
| expect(typeof callerRoot).toBe('string'); | |
| expect(fs.existsSync(path.join(callerRoot, 'package.json'))).toBe(true); | |
| const callerRoot = getCallerRoot(); | |
| expect(callerRoot).toBe(path.join(__dirname, '..')); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/root-utils/test/root-utils.test.js` around lines 27 - 29, The
current test only checks that a package.json exists at getCallerRoot(), which
can be satisfied by an unrelated root; instead read and parse the package.json
at path.join(callerRoot, 'package.json') and assert its "name" matches the
package name for this module (load the expected package name by requiring the
package.json adjacent to the test), e.g. const pkg =
JSON.parse(fs.readFileSync(path.join(callerRoot,'package.json'),'utf8'));
expect(pkg.name).toBe(require('../package.json').name); this strengthens the
getCallerRoot() assertion and prevents false positives.
|
|
||
| try { | ||
| assert.equal(getProcessRoot().endsWith('root-utils'), true); | ||
| expect(getProcessRoot().endsWith('root-utils')).toBe(true); |
There was a problem hiding this comment.
Prefer exact path assertion over suffix matching.
Line 69’s .endsWith('root-utils') is permissive and can pass for incorrect paths. Exact equality keeps this test deterministic.
Precise assertion
- expect(getProcessRoot().endsWith('root-utils')).toBe(true);
+ expect(getProcessRoot()).toBe(path.join(__dirname, '..'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(getProcessRoot().endsWith('root-utils')).toBe(true); | |
| expect(getProcessRoot()).toBe(path.join(__dirname, '..')); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/root-utils/test/root-utils.test.js` at line 69, Replace the
permissive suffix assertion on getProcessRoot() with an exact path equality
check: locate the test in root-utils.test.js that calls getProcessRoot() (the
expect(getProcessRoot().endsWith('root-utils')).toBe(true) line) and change it
to compare the full resolved path returned by getProcessRoot() to the exact
expected path string (or to a path constructed with path.resolve/path.join to
avoid platform issues) using toBe or toEqual so the test asserts precise
equality rather than just a suffix.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 133 133
Lines 8630 8628 -2
Branches 1460 1459 -1
=========================================
- Hits 8630 8628 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
no ref This change should have no user impact but makes it easier to migrate to TypeScript in the future.
EvanHahn
force-pushed
the
convert-rootutils-package-to-vitest
branch
from
6547e0f to
08fcfdd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/root-utils/test/root-utils.test.js (2)
27-29:⚠️ Potential issue | 🟠 MajorStrengthen caller-root assertion beyond file existence.
At Line 29, checking only for
package.jsonexistence is still too permissive and can validate the wrong root. Assert package identity (or exact expected path) to make this regression-proof.Suggested tightening
const callerRoot = getCallerRoot(); expect(typeof callerRoot).toBe('string'); - expect(fs.existsSync(path.join(callerRoot, 'package.json'))).toBe(true); + const pkg = JSON.parse(fs.readFileSync(path.join(callerRoot, 'package.json'), 'utf8')); + expect(pkg.name).toBe(require('../package.json').name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/root-utils/test/root-utils.test.js` around lines 27 - 29, Replace the loose existence check with a deterministic assertion: call getCallerRoot() (callerRoot) then read and parse the package.json at path.join(callerRoot, 'package.json') and assert a known field (e.g., packageJson.name === '<expected-package-name>') or assert the resolved callerRoot equals the exact expected root path; update the test expectations accordingly so it validates package identity rather than just file existence.
69-69:⚠️ Potential issue | 🟡 MinorUse exact root assertion instead of suffix match.
At Line 69,
.endsWith('root-utils')can still pass for incorrect paths. Use exact equality for determinism.Precise assertion
- expect(getProcessRoot().endsWith('root-utils')).toBe(true); + expect(getProcessRoot()).toBe(path.join(__dirname, '..'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/root-utils/test/root-utils.test.js` at line 69, Replace the fuzzy suffix check with a deterministic equality: compute an expectedRoot using Node's path.resolve from the test file location (e.g., path.resolve(__dirname, '..', ...) to reach the package root) and assert expect(getProcessRoot()).toBe(expectedRoot) instead of expect(...endsWith('root-utils')).toDo this in the test that calls getProcessRoot so the assertion compares exact full paths (use path.resolve and __dirname to derive expectedRoot).
🧹 Nitpick comments (2)
packages/root-utils/package.json (1)
31-32: Avoid runtime dependency drift in a test-framework migration.Line 31-32 broadens runtime dependency resolution (
caller,find-root). For a “no user impact” migration, this adds unnecessary behavior risk. If unintentional, revert to prior pinned values; if intentional, document why this is safe.Conservative revert
- "caller": "^1.0.1", - "find-root": "^1.1.0" + "caller": "1.1.0", + "find-root": "1.1.0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/root-utils/package.json` around lines 31 - 32, The dependency versions for "caller" and "find-root" were loosened which can cause runtime dependency drift; either revert "caller" and "find-root" to their previous pinned versions (restore exact versions instead of ^ ranges) so behavior remains identical, or add a clear comment in package.json and the PR description documenting the intentional reason this loosened range is safe (including test coverage/assertions that verify behavior) and mark the change as deliberate; update the package.json entries for the "caller" and "find-root" dependencies accordingly and run the lockfile update and tests to verify no regressions.packages/root-utils/test/root-utils.test.js (1)
46-57: Isolatecurrentdirectory setup to avoid test collisions.This still creates
currentunderprocess.cwd(), which can collide with parallel/leftover state. Prefer a temp cwd per test for deterministic behavior.Refactor sketch
- const currentDirectory = path.join(process.cwd(), 'current'); + const previousCwd = process.cwd(); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'root-utils-current-')); + const currentDirectory = path.join(tempDir, 'current'); const packageJSONPath = path.join(currentDirectory, 'package.json'); - fs.mkdirSync(currentDirectory); - fs.closeSync(fs.openSync(packageJSONPath, 'w')); - try { + process.chdir(tempDir); + fs.mkdirSync(currentDirectory); + fs.closeSync(fs.openSync(packageJSONPath, 'w')); expect(getProcessRoot()).toBe(currentDirectory); } finally { - fs.unlinkSync(packageJSONPath); - fs.rmdirSync(currentDirectory); + process.chdir(previousCwd); + if (fs.existsSync(packageJSONPath)) fs.unlinkSync(packageJSONPath); + if (fs.existsSync(currentDirectory)) fs.rmdirSync(currentDirectory); + fs.rmdirSync(tempDir); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/root-utils/test/root-utils.test.js` around lines 46 - 57, The test currently creates a 'current' dir under process.cwd(), risking collisions; change it to create a unique temporary directory (use fs.mkdtempSync with os.tmpdir() or fs.mkdtemp) and temporarily chdir into that temp dir before creating the 'current' folder and package.json; update the currentDirectory and packageJSONPath variables to be created inside that tempDir, call getProcessRoot() and assert, and in the finally block restore the original cwd and remove package.json, 'current', and the tempDir to ensure isolation and deterministic cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/root-utils/test/root-utils.test.js`:
- Around line 27-29: Replace the loose existence check with a deterministic
assertion: call getCallerRoot() (callerRoot) then read and parse the
package.json at path.join(callerRoot, 'package.json') and assert a known field
(e.g., packageJson.name === '<expected-package-name>') or assert the resolved
callerRoot equals the exact expected root path; update the test expectations
accordingly so it validates package identity rather than just file existence.
- Line 69: Replace the fuzzy suffix check with a deterministic equality: compute
an expectedRoot using Node's path.resolve from the test file location (e.g.,
path.resolve(__dirname, '..', ...) to reach the package root) and assert
expect(getProcessRoot()).toBe(expectedRoot) instead of
expect(...endsWith('root-utils')).toDo this in the test that calls
getProcessRoot so the assertion compares exact full paths (use path.resolve and
__dirname to derive expectedRoot).
---
Nitpick comments:
In `@packages/root-utils/package.json`:
- Around line 31-32: The dependency versions for "caller" and "find-root" were
loosened which can cause runtime dependency drift; either revert "caller" and
"find-root" to their previous pinned versions (restore exact versions instead of
^ ranges) so behavior remains identical, or add a clear comment in package.json
and the PR description documenting the intentional reason this loosened range is
safe (including test coverage/assertions that verify behavior) and mark the
change as deliberate; update the package.json entries for the "caller" and
"find-root" dependencies accordingly and run the lockfile update and tests to
verify no regressions.
In `@packages/root-utils/test/root-utils.test.js`:
- Around line 46-57: The test currently creates a 'current' dir under
process.cwd(), risking collisions; change it to create a unique temporary
directory (use fs.mkdtempSync with os.tmpdir() or fs.mkdtemp) and temporarily
chdir into that temp dir before creating the 'current' folder and package.json;
update the currentDirectory and packageJSONPath variables to be created inside
that tempDir, call getProcessRoot() and assert, and in the finally block restore
the original cwd and remove package.json, 'current', and the tempDir to ensure
isolation and deterministic cleanup.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/root-utils/package.jsonpackages/root-utils/test/.eslintrc.jspackages/root-utils/test/root-utils.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/root-utils/test/.eslintrc.js
Collaborator
|
Closing this out - migrated all packages to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
no ref
This change should have no user impact but makes it easier to migrate to TypeScript in the future.